Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update _sklearn.py #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ambader
Copy link

@ambader ambader commented Jul 30, 2021

check for horizon before X_fit is transformed

check for horizon before X_fit is transformed
Comment on lines +119 to +123
if self.optimize_for_horizon:
lenX=len(X)
else:
lenX=0
X_fit, y_fit = self._transform_data_to_tsmodel_input_format(self._X, self._y, lenX)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.optimize_for_horizon:
lenX=len(X)
else:
lenX=0
X_fit, y_fit = self._transform_data_to_tsmodel_input_format(self._X, self._y, lenX)
X_fit, y_fit = self._transform_data_to_tsmodel_input_format(self._X, self._y, len(X) if self.optimize_for_horizon else 0)

@MichalChromcak
Copy link
Collaborator

@ambader Could you bit more explain the idea behind the MR in the description?

@ambader
Copy link
Author

ambader commented Apr 5, 2022

Sure, I tried to solve the issue raised by @lucheroni (see). It been a while, so Im not totally sure about the details, but as far as i remember, the problem was that hcrystalball sets the length of periods to predict (the horizon) automatically. What I added is a way to check if the users tries to set another length and hands it over to the sklearn function, which can handle the new parameter.
I think you/the author was aware of this issue, because there is an attempt to integrate 'optimize_for_horizon' at line 155. Yet this is not working, as hcrystalball has already set the horizont length earlier.
You're welcome to ask if that wasn't clear, then I will have a closer look on it.

@MichalChromcak
Copy link
Collaborator

Yes, sorry for this big delay. Would you mind addind a test for this part?

@MichalChromcak
Copy link
Collaborator

Please see this notebook showing why we believe it is better idea to keep it as is.

02_ar_modelling_in_sklearn_patched.ipynb.zip

FYI
@pavelkrizek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants